UCT/IB/MLX5/DC: introducing dcs_hybrid policy #10138

roiedanino · 2024-09-09T11:14:09Z

What

Introducing a new dci allocation policy - dcs_hybrid which will behave the same as dcs_quota except using a dedicated HW dci when available instead of waiting for a dci from the pool to be available

Why ?

Utilizing hw resources instead of waiting for another dci, improves latency for certain message sizes

How ?

In case dci_alloc_or_create returned 0 - use the dedicated HW dci with a new dci channel.

src/uct/ib/mlx5/dc/dc_mlx5_ep.c

src/uct/ib/mlx5/dc/dc_mlx5_ep.h

yosefe · 2024-09-09T13:09:28Z

src/uct/ib/mlx5/dc/dc_mlx5_ep.h

+        uct_dc_mlx5_dci_pool_init_dci(iface, uct_dc_mlx5_ep_pool_index(ep),
+                                      UCT_DC_MLX5_HW_DCI);


does it mean HW DCS is the first DCI, that is most likely to be used?

It doesn't mean that, because in alloc_or_create we are allocating by the length of dcis array:

src/uct/ib/mlx5/dc/dc_mlx5_ep.h

yosefe · 2024-09-09T13:11:42Z

src/uct/ib/mlx5/dc/dc_mlx5_ep.h

+    if (!uct_dc_mlx5_iface_is_hybrid(iface)) {
+        return UCS_ERR_NO_RESOURCE;
+    }
+
+    if (ep->dci == UCT_DC_MLX5_HW_DCI) {
+        return UCS_OK;
+    }
+
+    if (uct_dc_mlx5_iface_dci_has_tx_resources(iface, UCT_DC_MLX5_HW_DCI)) {


can we try to reduce number of branches on the fast path?

roiedanino · 2024-09-18T13:39:52Z

Add hw_dci attribute to iface which will be -1 if hybrid was not selected

src/uct/ib/mlx5/dc/dc_mlx5.h

brminich

can you create some DC specific gtests for this new mode?

brminich · 2024-09-19T16:40:34Z

src/uct/ib/mlx5/dc/dc_mlx5.c

@@ -901,7 +903,7 @@ uct_dc_mlx5_iface_dcis_create(uct_dc_mlx5_iface_t *iface,

    ucs_array_length(&iface->tx.dcis) = 0;

-    status = uct_dc_mlx5_iface_create_dci(iface, 0, 0);
+    status = uct_dc_mlx5_iface_create_dci(iface, 0, 0, 1);


why just one channel and not self->tx.num_dci_channels?

It doesn't matter as this dci is only created to query bb_max and then destroyed (lines 913-914)

ic, probably need to change the name of this func, as it is quite confusing currently

src/uct/ib/mlx5/dc/dc_mlx5_ep.h

brminich · 2024-09-19T16:50:27Z

src/uct/ib/mlx5/dc/dc_mlx5_ep.h

+    if (uct_dc_mlx5_iface_is_dci_shared(iface) ||
+        uct_dc_mlx5_is_hw_dci(iface, ep->dci)) {


seems these two check are frequently used together. separate func is worth creating

yosefe · 2024-09-22T12:15:44Z

src/uct/ib/mlx5/dc/dc_mlx5_ep.h

+    return uct_dc_mlx5_iface_is_dci_shared(iface) ||
+           uct_dc_mlx5_is_hw_dci(iface, dci);


can we optimize this to be a single branch? it is used in uct_dc_mlx5_iface_dci_get

I guess replacing || with +?

both these checks are checking the policy field, maybe we can a bitmap based check or something like "policy >= UCT_DC_TX_POLICY_DCS_HYBRID" ?

is_hw_dci just checks dci == iface->tx.hybrid_hw_dci so >= DCS_HYBRID won't help, we can just use bitwise | between is_shared and is_hw_dci if that's somehow better

yosefe · 2024-09-25T13:40:46Z

src/uct/ib/mlx5/dc/dc_mlx5.h

@@ -212,6 +212,7 @@ typedef struct uct_dc_dci {
    uint8_t                       path_index; /* Path index */
    uint8_t                       next_channel_index; /* next DCI channel index
                                                         to be used by EP */
+    uint8_t                       is_shared; /* Indicates that this specific dci is shared, regardless of policy*/


space before *

line seems too long

maybe make it uint8_t flags?

It seems like we can't avoid that extra branch because now we don't know if dci is UCT_DC_MLX5_EP_NO_DCI, resulting in sanitizer failure (accessing dcis[255])

yosefe · 2024-09-25T13:41:50Z

src/uct/ib/mlx5/dc/dc_mlx5_ep.h

@@ -366,6 +365,8 @@ uct_dc_mlx5_dci_pool_init_dci(uct_dc_mlx5_iface_t *iface, uint8_t pool_index,
        return status;
    }

+    dci->is_shared  = uct_dc_mlx5_iface_is_policy_shared(iface) ||


set it later - initialize according to fields order in the struct

src/uct/ib/mlx5/dc/dc_mlx5.h

yosefe · 2024-09-26T15:13:28Z

src/uct/ib/mlx5/dc/dc_mlx5_ep.h

    ucs_arbiter_t *waitq;
    uct_rc_txqp_t *txqp;
    int16_t available;

    ucs_assert(!iface->super.super.config.tx_moderation);

-    if (uct_dc_mlx5_iface_is_dci_shared(iface)) {
+    if (ep->dci == UCT_DC_MLX5_EP_NO_DCI) {


remove assert+comment in line 745?

pls also update or remove the comment in that line

Signed-off-by: Roie Danino <[email protected]>

roiedanino requested a review from yosefe September 9, 2024 11:14

roiedanino self-assigned this Sep 9, 2024

roiedanino requested a review from shasson5 September 9, 2024 11:53

yosefe reviewed Sep 9, 2024

View reviewed changes

roiedanino requested a review from yosefe September 10, 2024 10:01

yosefe reviewed Sep 19, 2024

View reviewed changes

src/uct/ib/mlx5/dc/dc_mlx5.h Outdated Show resolved Hide resolved

brminich reviewed Sep 19, 2024

View reviewed changes

roiedanino requested review from brminich and yosefe September 22, 2024 11:26

yosefe reviewed Sep 22, 2024

View reviewed changes

roiedanino requested a review from yosefe September 24, 2024 10:40

yosefe reviewed Sep 25, 2024

View reviewed changes

roiedanino requested a review from yosefe September 26, 2024 12:07

yosefe reviewed Sep 26, 2024

View reviewed changes

roiedanino requested a review from yosefe September 29, 2024 08:02

yosefe approved these changes Sep 29, 2024

View reviewed changes

UCT/IB/MLX5/DC: Introduce dcs_hybrid policy

e558c11

Signed-off-by: Roie Danino <[email protected]>

roiedanino force-pushed the hw-dcs-2 branch from 9f006eb to e558c11 Compare September 29, 2024 14:04

roiedanino enabled auto-merge September 29, 2024 14:04

roiedanino merged commit 13af469 into openucx:master Sep 30, 2024
141 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCT/IB/MLX5/DC: introducing dcs_hybrid policy #10138

UCT/IB/MLX5/DC: introducing dcs_hybrid policy #10138

roiedanino commented Sep 9, 2024 •

edited

Loading

yosefe Sep 9, 2024

roiedanino Sep 9, 2024

yosefe Sep 9, 2024

roiedanino commented Sep 18, 2024

brminich left a comment •

edited

Loading

brminich Sep 19, 2024

roiedanino Sep 19, 2024

brminich Sep 19, 2024

brminich Sep 19, 2024

yosefe Sep 22, 2024

roiedanino Sep 22, 2024 •

edited

Loading

yosefe Sep 23, 2024

roiedanino Sep 24, 2024

yosefe Sep 25, 2024

roiedanino Sep 26, 2024

yosefe Sep 25, 2024

yosefe Sep 26, 2024

yosefe Sep 29, 2024

		uct_dc_mlx5_dci_pool_init_dci(iface, uct_dc_mlx5_ep_pool_index(ep),
		UCT_DC_MLX5_HW_DCI);

		if (uct_dc_mlx5_iface_is_dci_shared(iface) \|\|
		uct_dc_mlx5_is_hw_dci(iface, ep->dci)) {

		return uct_dc_mlx5_iface_is_dci_shared(iface) \|\|
		uct_dc_mlx5_is_hw_dci(iface, dci);

UCT/IB/MLX5/DC: introducing dcs_hybrid policy #10138

UCT/IB/MLX5/DC: introducing dcs_hybrid policy #10138

Conversation

roiedanino commented Sep 9, 2024 • edited Loading

What

Why ?

How ?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roiedanino commented Sep 18, 2024

brminich left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roiedanino Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roiedanino commented Sep 9, 2024 •

edited

Loading

brminich left a comment •

edited

Loading

roiedanino Sep 22, 2024 •

edited

Loading